-
-
Notifications
You must be signed in to change notification settings - Fork 234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add debounce to ScreenshotWidget
#2368
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2368 +/- ##
==========================================
+ Coverage 84.78% 85.05% +0.27%
==========================================
Files 253 79 -174
Lines 9070 2797 -6273
==========================================
- Hits 7690 2379 -5311
+ Misses 1380 418 -962 ☔ View full report in Codecov by Sentry. |
iOS Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a7acb24 | 1296.71 ms | 1317.69 ms | 20.98 ms |
1e094d3 | 1237.54 ms | 1243.51 ms | 5.97 ms |
ca9f398 | 1227.29 ms | 1236.20 ms | 8.92 ms |
6f3717a | 1259.84 ms | 1280.90 ms | 21.06 ms |
0118295 | 1211.31 ms | 1227.02 ms | 15.71 ms |
2e1e4ae | 1254.41 ms | 1278.55 ms | 24.14 ms |
136c365 | 1248.12 ms | 1277.33 ms | 29.20 ms |
689d2fd | 1257.71 ms | 1265.16 ms | 7.45 ms |
d7758e8 | 1271.69 ms | 1288.08 ms | 16.39 ms |
d61cecf | 1276.96 ms | 1290.02 ms | 13.06 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a7acb24 | 8.16 MiB | 9.17 MiB | 1.01 MiB |
1e094d3 | 8.29 MiB | 9.37 MiB | 1.08 MiB |
ca9f398 | 8.38 MiB | 9.74 MiB | 1.36 MiB |
6f3717a | 8.33 MiB | 9.62 MiB | 1.29 MiB |
0118295 | 8.32 MiB | 9.38 MiB | 1.05 MiB |
2e1e4ae | 8.34 MiB | 9.65 MiB | 1.31 MiB |
136c365 | 8.38 MiB | 9.75 MiB | 1.37 MiB |
689d2fd | 8.10 MiB | 9.16 MiB | 1.06 MiB |
d7758e8 | 8.15 MiB | 9.12 MiB | 989.76 KiB |
d61cecf | 8.10 MiB | 9.18 MiB | 1.08 MiB |
Android Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
117d988 | 376.32 ms | 450.85 ms | 74.53 ms |
b98109e | 296.46 ms | 362.68 ms | 66.22 ms |
90a08ea | 477.25 ms | 534.10 ms | 56.85 ms |
40680d3 | 323.55 ms | 390.29 ms | 66.73 ms |
deaeece | 347.42 ms | 381.10 ms | 33.68 ms |
95d0636 | 301.46 ms | 357.98 ms | 56.52 ms |
24b6e60 | 440.64 ms | 557.96 ms | 117.32 ms |
c328ffc | 394.35 ms | 480.94 ms | 86.59 ms |
26e955b | 369.52 ms | 458.60 ms | 89.07 ms |
8d64376 | 302.88 ms | 356.84 ms | 53.96 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
117d988 | 6.33 MiB | 7.26 MiB | 947.03 KiB |
b98109e | 6.06 MiB | 7.03 MiB | 993.53 KiB |
90a08ea | 6.49 MiB | 7.55 MiB | 1.06 MiB |
40680d3 | 6.06 MiB | 7.03 MiB | 989.25 KiB |
deaeece | 5.94 MiB | 6.96 MiB | 1.02 MiB |
95d0636 | 6.16 MiB | 7.14 MiB | 1007.32 KiB |
24b6e60 | 6.33 MiB | 7.26 MiB | 950.14 KiB |
c328ffc | 6.35 MiB | 7.42 MiB | 1.07 MiB |
26e955b | 6.27 MiB | 7.20 MiB | 956.49 KiB |
8d64376 | 5.94 MiB | 6.96 MiB | 1.02 MiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to use it, but due to the callback nature it didn't fit with the event processors. Will take a look if it can be adapted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also they work a bit different, the first call always goes through in this PR, whereas the Debouncer
would not call it. I also don't delay, but rather bail out on consecutive calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay I understand. Would it make sense, to extend the current debouncer with some parameters, to behave it that way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't feel that it is worth the effort, as it is different behaviour and we only use it in this one case.
Closing due to discussion here: #2371 (comment) |
📜 Description
Debounce screenshots made by sentry to avoid being removed by iOS watchdog.
💡 Motivation and Context
Relates to #2360
💚 How did you test it?
Unit tests.
📝 Checklist
sendDefaultPii
is enabled